Skip to content

Include changeset id in deletion#35

Closed
gmaclennan wants to merge 1 commit intomasterfrom
changeset-deletions
Closed

Include changeset id in deletion#35
gmaclennan wants to merge 1 commit intomasterfrom
changeset-deletions

Conversation

@gmaclennan
Copy link
Copy Markdown
Member

@gmaclennan gmaclennan commented Feb 4, 2017

@noffle could you review and add a test here (see the failing test in osm-p2p-server https://github.com/digidem/osm-p2p-db/compare/changeset-deletions) attempts to fix #34

@gmaclennan gmaclennan requested a review from hackergrrl February 4, 2017 02:20
@hackergrrl
Copy link
Copy Markdown
Contributor

What do you think about just having fields.v set to opts.value directly? This makes it more versatile for the future.

@gmaclennan
Copy link
Copy Markdown
Member Author

That makes sense to me. I'm not sure about @substack's original motivation for choosing this API.

@hackergrrl
Copy link
Copy Markdown
Contributor

Can you also update the osm.del() calls we make to set opts.value.changeset?

Comment thread index.js
fields.refs.push.apply(fields.refs, v.refs)
}
})
if (opts.value && opts.value.changeset) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about just checking for opts.changeset?

@hackergrrl
Copy link
Copy Markdown
Contributor

Let's nix this PR and update osm-p2p-db#del() to accept a value on deletions, just like PUTs, like we discussed in meatspace.

@gmaclennan
Copy link
Copy Markdown
Member Author

closing in favour of #46

@gmaclennan gmaclennan closed this Jun 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Changeset is not stored on deleted elements

2 participants